internal/efi: Add HostEnvironment bits for efi/preinstall #314
Conversation
chrisccoulson
force-pushed
the
internal-efi-add-preinstall-host-environment
branch
3 times, most recently
from
0d5a105 to
dbf3805
Compare
This extends the HostEnvironment interface to accommodate efi/preinstall, to make it a superset of the one that's already exposed by the efi package. The intention is that this superset won't be exposed for now, other than for unit testing - the efi package still only exposes the existing subset. The defaultEnvImpl in has been updated to provide a default implementation of it, and the internal/efitest package has been updated to be able to mock the new parts of it.
chrisccoulson
force-pushed
the
internal-efi-add-preinstall-host-environment
branch
from
dbf3805 to
1bc68cf
Compare
internal/efi/env.go
Outdated
| HasCPUIDFeature(feature uint64) bool | ||
|
|
||
| // ReadMSRs reads the value of the specified MSR for all CPUs, | ||
| // returning a map of the result for all CPU numbers |
There was a problem hiding this comment.
maybe "a map of the results keyed by the CPU numbers" would be clearer here?
There was a problem hiding this comment.
I've updated this.
| return nil, ErrNoDeviceAttribute | ||
| } | ||
|
|
||
| f, err := os.Open(filepath.Join(d.path, attr)) | ||
| switch { | ||
| case os.IsNotExist(err): | ||
| return nil, ErrNoDeviceAttribute |
There was a problem hiding this comment.
these error path are no tested
There was a problem hiding this comment.
I've added a test for this
| return nil, err | ||
| } | ||
| if !fi.Mode().IsRegular() { | ||
| return nil, ErrNoDeviceAttribute |
There was a problem hiding this comment.
I've added a test for this
| switch { | ||
| case os.IsNotExist(err): | ||
| // it's ok to have no devices for the specified class | ||
| return nil, nil |
There was a problem hiding this comment.
I've added a test for this
efi/shim_test.go
Outdated
| @@ -400,7 +400,7 @@ func (s *shimSuite) TestShimSbatPolicyLatestUnset(c *C) { | |||
|
|
|||
| c.Assert(visitor.varModifiers, HasLen, 1) | |||
|
|
|||
| collector := NewVariableSetCollector(efitest.NewMockHostEnvironment(nil, nil)) | |||
| collector := NewVariableSetCollector(efitest.NewMockHostEnvironment(efitest.MockVars{}, nil)) | |||
There was a problem hiding this comment.
what's the reason for the need of this change and other similar ones?
There was a problem hiding this comment.
Ah, it was because I made a change to MockHostEnvironment.VarContext to not attach a efi.VarsBackend if the supplied efitest.MockVars map is nil, which resulted in anything that ends up calling efi.ReadVariable crashing. I reverted the change to how it worked before - MockHostEnvironment.VarContext will always add a efi.VarsBackend even if the supplied efitest.MockVars is nil. This does mean that anything which calls efi.ReadVariable ends up reading keys from a nil map, but that seems to be ok in go (it's only adding entries to a nil map that causes a panic)
The previously committed change only added an implementation of efi.VarsBackend to the supplied context if the supplied efitest.MockVars was not nil, but this resulted in existing tests having to explicitly create an empty efitest.MockVars. Revert to the existing behaviour where we add an implementation of efi.VarsBackend to the supplied context in all cases. Although the implementation is a nil map, that is ok for reading (it's only adding keys to a nil map that causes go to panic).
This extends the internal
HostEnvironmentinterface to accommodateefi/preinstall, to make it a superset of the one that's already exposed by theefipackage. The intention is that this superset won't be exposed for now,other than for unit testing - the
efipackage still only exposes the existingsubset with methods for overriding EFI variables and the TCG log.
The defaultEnvImpl in has been updated to provide a default implementation of
it, and the
internal/efitestpackage has been updated to be able to mock thenew parts of it.